fix: use certificate fingerprints to deduplicate TLS certs #25779
Conversation
Signed-off-by: nitishfy <[email protected]>
🔴 Preview Environment stopped on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
Signed-off-by: nitishfy <[email protected]>
| // certFingerprintSHA256 returns the SHA256 fingerprint of the given X.509 certificate. | ||
| // The fingerprint is returned as a lowercase hexadecimal string. | ||
| func certFingerprintSHA256(cert *x509.Certificate) string { | ||
| sum := sha256.Sum256(cert.Raw) |
There was a problem hiding this comment.
This is just a question. Using cert.Raw treats certificate renewals with the same public key as distinct certificates. Is this intentional?
There was a problem hiding this comment.
Yes, this is intentional. The goal of this change is to deduplicate identical certificate data, not to collapse logically related certificates.
|
This looks good to me! Since I agree that certificate renewal is a prominent use case for subject collisions, how about adding expiration dates into |
| for _, entry := range pems { | ||
| x509cert, err := certutil.DecodePEMCertificateToX509(entry) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Consider wrapping the error with the index so users know which PEM block failed.
| for _, entry := range pems { | |
| x509cert, err := certutil.DecodePEMCertificateToX509(entry) | |
| if err != nil { | |
| return nil, err | |
| } | |
| for i, entry := range pems { | |
| x509cert, err := certutil.DecodePEMCertificateToX509(entry) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to decode certificate at index %d: %w", i, err) | |
| } |
| errors.CheckError(err) | ||
|
|
||
| fmt.Printf( | ||
| "Created/updated TLS certificate entry for repository server %s with %d unique PEM certificates\n", |
There was a problem hiding this comment.
can we differentiate the message between creation and update to give the user a more exact information?
| errors.CheckError(err) | ||
|
|
||
| fmt.Printf( | ||
| "Created/updated TLS certificate entry for repository server %s with %d unique PEM certificates\n", |
There was a problem hiding this comment.
Also minor but why is this broken in two lines?
| // Two certificates are considered duplicates if their SHA256 fingerprints match. | ||
| // The function returns a slice of unique certificates in the original order. | ||
| // If any certificate cannot be decoded into X.509 format, an error is returned. | ||
| func deduplicatePEMCertificates(pems []string) ([]string, error) { |
There was a problem hiding this comment.
Would you consider to just log a warning if a certificate cannot be decoded, and continue with the next one instead of returning an error?
| fingerprint := certFingerprintSHA256(x509cert) | ||
|
|
||
| if _, exists := fingerprintMap[fingerprint]; exists { | ||
| fmt.Printf( |
There was a problem hiding this comment.
Please use fmt.Fprintf(os.Stderr, .... instead
this will allow users to capture output consistently
4 participants
The existing
argocd cert add-tlscommand had two key issues with handling TLS certificates:Previously, the command rejected certificates with the same subject, even if the actual certificate data was different (e.g., renewed or rotated certificates). This caused valid certificates to be incorrectly skipped, preventing users from storing multiple valid TLS certs for the same server.
The command displayed the number of RepositoryCertificate items returned by the API as the number of certificates added. Since the API returns a single object per server regardless of how many PEM blocks are included, the message could misleadingly indicate 0 certificates were stored, confusing users.
After
Checklist: